[202605][intfsorch]: Retry explicit router interface VRF rehome#4765
Closed
Xichen96 wants to merge 2 commits into
Closed
[202605][intfsorch]: Retry explicit router interface VRF rehome#4765Xichen96 wants to merge 2 commits into
Xichen96 wants to merge 2 commits into
Conversation
…ce removal Keep the existing whole-interface DEL retry fenced after prefix deletion. Standard route, neighbor, and neighbor-next-hop creation now retries instead of attaching to the RIF being removed, while teardown continues to use the raw RIF lookup. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b6b568a1-5d2a-4309-b1ac-ef21ca155079 Signed-off-by: Xichen Lin <lukelin0907@gmail.com>
Retain an explicit vrf_name SET until prefixes and RIF references drain, then replace the RIF in the requested VRF. Preserve interface attributes, block replacement prefix additions, and roll back to the old RIF if replacement creation fails. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b6b568a1-5d2a-4309-b1ac-ef21ca155079 Signed-off-by: Xichen Lin <lukelin0907@gmail.com>
Collaborator
|
/azp run |
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
This was referenced Jul 19, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
Backport to the 202605 branch of the IntfsOrch “explicit VRF rehome retry” behavior, ensuring interface VRF changes are retained and retried until prefixes and dependent objects drain, and that newly created dependencies are fenced while the replacement RIF is pending.
Changes:
- Add
m_pendingVrfUpdatesfencing and VRF rehome replacement/rollback logic inIntfsOrch, including preservation of key RIF creation attributes (MAC, loopback action, MPLS/NAT state viaPort). - Update RouteOrch/NeighOrch to honor the new dependency fence via
getRouterIntfsIdForNewDependency()and to propagateaddNextHop()failure. - Extend mock-test coverage for VRF rehome drain/retry, attribute preservation, partial updates, and rollback-on-create-failure.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/mock_tests/intfsorch_ut.cpp | Adds focused unit tests for VRF rehome retry/drain behavior, attribute preservation, and rollback. |
| orchagent/routeorch.cpp | Switches some interface NH lookups to the fenced API and propagates neighbor next-hop creation failure. |
| orchagent/neighorch.cpp | Uses fenced RIF lookup for new dependency creation and returns retry when RIF is unavailable. |
| orchagent/intfsorch.h | Extends IntfsEntry with preserved attributes and introduces getRouterIntfsIdForNewDependency() plus pending-update tracking. |
| orchagent/intfsorch.cpp | Implements pending VRF update fencing, VRF rehome replacement/rollback, explicit-field handling, and attribute preservation updates. |
Comments suppressed due to low confidence (1)
orchagent/routeorch.cpp:2103
addRoute()now correctly usesgetRouterIntfsIdForNewDependency()for interface next-hops, but other route-programming paths still appear to bypass the dependency fence. In particular,addRoutePost()still callsm_intfsOrch->getRouterIntfsId(nexthop.alias)(around routeorch.cpp:2450) for interface NH validation, which can allow a route update to proceed and take a new ref on a RIF while an interface is inm_pendingVrfUpdates/m_removingIntfses. That undermines the intended “new dependencies wait for replacement RIF” behavior during VRF rehome/removal.
Recommend switching the interface NH lookup in addRoutePost() (and any similar validation paths) to getRouterIntfsIdForNewDependency() so all route additions consistently honor the fence.
next_hop_id = m_intfsOrch->getRouterIntfsIdForNewDependency(nexthop.alias);
/* rif is not created yet */
if (next_hop_id == SAI_NULL_OBJECT_ID)
{
SWSS_LOG_INFO("Failed to get next hop %s for %s",
nextHops.to_string().c_str(), ipPrefix.to_string().c_str());
return false;
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What I did
Backported the explicit physical-interface VRF rehome retry to 202605.
SETinm_toSyncwhile prefixes or RIF references still exist.SETs and the guarded route/neighbor dependencies wait for the replacement RIF.DELs to continue using the old RIF so it can drain.Portstate used to create the RIF, including loopback action, MPLS state, NAT zone, and custom RIF MAC.vrf_nameas another move.SETqueued for retry; logical VRF/refcount state changes only after replacement succeeds.Why I did it
Whole-interface deletion already handled cross-key ordering correctly: if it arrived before the final prefix
DEL, it returnedfalse, stayed queued, and retried after dependencies drained.An explicit VRF-changing
SETdid not. When a physical interface already had a RIF, the old create path returned success without moving it. If the consumer visited:the first
SETcould be consumed while the old prefix still existed. The laterDELremoved the dependency, but there was no VRF request left to revisit, so the RIF and IP2Me route remained in the old virtual router.The fix does not require APP_DB producers to coordinate a global barrier or deliver keys in lifecycle order. IntfsOrch retains the unresolved VRF request, lets old dependencies drain, performs the replacement, and then allows queued additions to resolve against the new RIF. This is local quiescence and retry, not a universal priority queue.
If ProducerStateTable coalesces multiple operations on the same key before the consumer receives them, the consumer cannot reconstruct those lost intermediate events; this PR does not claim to solve that separate StateTable lifecycle-coalescing case.
How I verified it
Added focused mock coverage for:
SETretained across prefix/reference drain and prefix re-add on the new RIF.SETwithoutvrf_name.The earlier combined 202605 branch containing this implementation completed the amd64, ARM, ASAN, Docker, and Trixie compile stages in Azure build
1168367. Itsvstestjobs stopped before executing tests because the workers lackedpip3.An earlier combined 202605 candidate, hardware build
1167872, passed traced and production 1x/5x runs and all ten production 10x test bodies. This smaller stacked commit adds state-preservation and rollback hardening after that run, so exact-commit validation is provided by this PR's new CI.Details if related
Depends on the 202605 deletion-fence PR: #4747
Master PR: #4764